-
Notifications
You must be signed in to change notification settings - Fork 380
fix(skeleton): remove dead gift card client-side state tracking #3426
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Oxygen deployed a preview of your
Learn more about Hydrogen's GitHub integration. |
d7e2cc3 to
ba7a43d
Compare
c79dc17 to
64be556
Compare
| useEffect(() => { | ||
| if (giftCardAddFetcher.data) { | ||
| giftCardCodeInput.current!.value = ''; | ||
| } | ||
| }, [giftCardAddFetcher.data]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im not a fan of this either but i guess there aren’t callbacks for fetcher to reset the form
we could pass a ref to the form and call form.reset() that would give me some peace of mind but overall they do the same so whatever, lets not change for the sake of changing it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code looks harmless, BUT e2e tests are failing in CI and just hang forever locally – investigating which branch originated this
The CartGiftCard component maintained a useRef that accumulated gift card codes client-side, but this ref was never read - only written to. This created potential for "zombie" bugs where the client and server state could diverge (the ref only grew, never shrunk when cards were removed). Since the GiftCardCodesAdd action handles everything server-side and the UI displays gift cards directly from the GraphQL response (cart.appliedGiftCards), this client-side tracking was redundant and potentially problematic. Removed: - appliedGiftCardCodes useRef (dead code - write-only) - saveAppliedCode function (populated unused ref) - Render-prop pattern with side effects in AddGiftCardForm - Unused FetcherWithComponents import
64be556 to
79f5545
Compare
The CartGiftCard component maintained a useRef that accumulated gift card codes client-side, but this ref was never read - only written to. This created potential for "zombie" bugs where the client and server state could diverge (the ref only grew, never shrunk when cards were removed).
Since the GiftCardCodesAdd action handles everything server-side and the UI displays gift cards directly from the GraphQL response (cart.appliedGiftCards), this client-side tracking was redundant and potentially problematic.
Removed:
WHY are these changes introduced?
Fixes #0000
WHAT is this pull request doing?
HOW to test your changes?
Post-merge steps
Checklist